Address some pixel local reco PR review comments #575
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
How much do we want to review our updates to the CMSSW PR before merging? (my feeling is that the changes are easier to review here than there) |
fwyzard
reviewed
Nov 21, 2020
fwyzard
reviewed
Nov 21, 2020
Comment on lines
30
to
33
| uint32_t const *moduleStart() const { return moduleStart_d.get(); } | ||
| uint32_t const *clusInModule() const { return clusInModule_d.get(); } | ||
| uint32_t const *moduleId() const { return moduleId_d.get(); } | ||
| uint32_t const *clusModuleStart() const { return clusModuleStart_d.get(); } |
There was a problem hiding this comment.
This (and the equivalent in SiPixelDigisCUDA.h) is the only change I'm not very fond of.
I've seen the comment by Slava - but, if std::vector can happily have both begin() and cbegin() etc. I don't see why we cannot have the same here.
However, could you remind me why we actually need c_moduleStart() etc. ?
What I mean is, if somebody wants a const*, why not just write
uint32_t const* start = clusters.moduleStart();instead of
uint32_t const* start = clusters.c_moduleStart();?
Moreover, all use cases I've found are in SiPixelRawToClusterGPUKernel.cu, and are used to pass a const * argument to a kernel.
Using .moduleInd() instead of .c_moduleInd() etc. should work just fine in this case, and the pointer would still be const in the kernel function.
Am I missing something else ?
There was a problem hiding this comment.
Using non-const clusModuleStart() would indeed work. The idea of c_clusModuleStart() is for a case where the returned pointer is passed directly to a function along func(clusters.moduleStart(), ...) and the caller wants to ensure that the function gets a pointer-to-const. The func() may today take a pointer-to-const, but someone could change the function to take a pointer-to-non-const and modify the contents of the pointed-to-memory. The c_clusModuleStart() approach (or explicit const_cast) would help to catch that mismatch with a compilation error.
Written that, I suppose for this particular case the question is more academic, given that all the uses of non-const clusters is in a single file (plus a few #includes) and that this pattern for SoA would better not spread much. So I can change to the other way as well (but would really like to avoid changing again after that :) ).
There was a problem hiding this comment.
The last three commits show the impact of dropping c_ prefix. If we go with this approach, I'll squash them into the respective commits having other changes of the affected files.
|
@makortel thank you for this. |
makortel
force-pushed
the
patatrackPixelLocalRecoReview
branch
from
fb30898
to
fe630f4
Compare
makortel
changed the title
Now could be a good moment, thanks. I'm not planning to proceed further for now (because of time constraints). |
makortel
changed the title
|
Did the validation get stuck? |
|
The validation failed on .502, among the others, with something like To fix this what I found helpful was:
|
|
Thanks, so the dictionary generation failed somehow |
makortel
force-pushed
the
patatrackPixelLocalRecoReview
branch
from
fe630f4
to
93150d2
Compare
|
Ok, it was pretty clear why the dictionaries were not generated. Could you try again now? |
|
Now there is a merge conflict from merge of #577 |
Should have been removed as part of cms-patatrack#100.
- Remove commented out default constructor and private: from DeviceConstView
* This is perhaps the best compromise between non-default
constructors not being preferred for device allocations, and the
use case in SiPixelRecHitSoAFromLegacy (for the expected life time
of this class)
- Remove const getters without c_ prefix
- Improve constructor parameter name
- Use more initializer list
- initialize nClusters_h
- Use type alias - Remove unnecessary method - Use more initializer list
- Remove unnecessary methods - Remove commented out default constructor and private: from DeviceConstView - Add comments for remaining SiPixelDigisCUDA member arrays
…elRawData, rename classes
- Remove redundant assert - Move constructor inline
- Remove redundant assert - Add comments
makortel
force-pushed
the
patatrackPixelLocalRecoReview
branch
from
93150d2
to
a15f89e
Compare
|
Rebased on top of 11_2_X_Patatrack to fix the conflict (testing building now, will take a while) |
|
thanks, I didn't even manage to ask ;-) |
|
the error is
which shouldn't be related to these changes... will have a look |
|
Validation summaryReference release CMSSW_11_2_0_pre10 at 6c149b2 Validation plots/RelValTTbar_14TeV/CMSSW_11_2_0_pre7-PU_112X_mcRun3_2021_realistic_v8-v1/GEN-SIM-DIGI-RAW
/RelValZMM_14/CMSSW_11_2_0_pre7-112X_mcRun3_2021_realistic_v8-v2/GEN-SIM-DIGI-RAW
/RelValZEE_14/CMSSW_11_2_0_pre7-112X_mcRun3_2021_realistic_v8-v1/GEN-SIM-DIGI-RAW
Validation plots (CPU vs GPU)/RelValTTbar_14TeV/CMSSW_11_2_0_pre7-PU_112X_mcRun3_2021_realistic_v8-v1/GEN-SIM-DIGI-RAW
/RelValZMM_14/CMSSW_11_2_0_pre7-112X_mcRun3_2021_realistic_v8-v2/GEN-SIM-DIGI-RAW
/RelValZEE_14/CMSSW_11_2_0_pre7-112X_mcRun3_2021_realistic_v8-v1/GEN-SIM-DIGI-RAW
Throughput plots/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53
logs and
|
|
Looks good to me. |
fwyzard
added a commit
that referenced
this pull request
Nov 27, 2020
Remove SiPixelDigiHeterogeneousConverter as obsolete, should have been removed as part of #100. Address review comments for SiPixelClustersCUDA: - remove commented out default constructor and private: from DeviceConstView; this is perhaps the best compromise between non-default constructors not being preferred for device allocations, and the use case in SiPixelRecHitSoAFromLegacy (for the expected life time of this class) - remove const getters with c_ prefix - improve constructor parameter name - use more initializer list - initialize nClusters_h Address review comments for SiPixelDigiErrorsCUDA: - use type alias - remove const getters with c_ prefix and other unnecessary methods - use more initializer list Address review comments for SiPixelDigisCUDA: - remove const getters with c_ prefix and other unnecessary methods - remove commented out default constructor and private: from DeviceConstView - add comments for remaining SiPixelDigisCUDA member arrays Move PixelErrorsCompact and SiPixelDigiErrorsSoa to DataFormats/SiPixelRawData, rename classes Address review comments for SiPixelErrorsSoA - remove redundant assert - move constructor inline Address review comments for SiPixelDigisSoA - remove redundant assert - add comments Enable if constexpr also for CUDA in TrackingRecHit2DHeterogeneous Move dictionary of HostProduct<unsigned int[]> to CUDADataFormats/Common
fwyzard
pushed a commit
that referenced
this pull request
Dec 25, 2020
Remove SiPixelDigiHeterogeneousConverter as obsolete, should have been removed as part of #100. Address review comments for SiPixelClustersCUDA: - remove commented out default constructor and private: from DeviceConstView; this is perhaps the best compromise between non-default constructors not being preferred for device allocations, and the use case in SiPixelRecHitSoAFromLegacy (for the expected life time of this class) - remove const getters with c_ prefix - improve constructor parameter name - use more initializer list - initialize nClusters_h Address review comments for SiPixelDigiErrorsCUDA: - use type alias - remove const getters with c_ prefix and other unnecessary methods - use more initializer list Address review comments for SiPixelDigisCUDA: - remove const getters with c_ prefix and other unnecessary methods - remove commented out default constructor and private: from DeviceConstView - add comments for remaining SiPixelDigisCUDA member arrays Move PixelErrorsCompact and SiPixelDigiErrorsSoa to DataFormats/SiPixelRawData, rename classes Address review comments for SiPixelErrorsSoA - remove redundant assert - move constructor inline Address review comments for SiPixelDigisSoA - remove redundant assert - add comments Enable if constexpr also for CUDA in TrackingRecHit2DHeterogeneous Move dictionary of HostProduct<unsigned int[]> to CUDADataFormats/Common
fwyzard
pushed a commit
that referenced
this pull request
Dec 29, 2020
Remove SiPixelDigiHeterogeneousConverter as obsolete, should have been removed as part of #100. Address review comments for SiPixelClustersCUDA: - remove commented out default constructor and private: from DeviceConstView; this is perhaps the best compromise between non-default constructors not being preferred for device allocations, and the use case in SiPixelRecHitSoAFromLegacy (for the expected life time of this class) - remove const getters with c_ prefix - improve constructor parameter name - use more initializer list - initialize nClusters_h Address review comments for SiPixelDigiErrorsCUDA: - use type alias - remove const getters with c_ prefix and other unnecessary methods - use more initializer list Address review comments for SiPixelDigisCUDA: - remove const getters with c_ prefix and other unnecessary methods - remove commented out default constructor and private: from DeviceConstView - add comments for remaining SiPixelDigisCUDA member arrays Move PixelErrorsCompact and SiPixelDigiErrorsSoa to DataFormats/SiPixelRawData, rename classes Address review comments for SiPixelErrorsSoA - remove redundant assert - move constructor inline Address review comments for SiPixelDigisSoA - remove redundant assert - add comments Enable if constexpr also for CUDA in TrackingRecHit2DHeterogeneous Move dictionary of HostProduct<unsigned int[]> to CUDADataFormats/Common
fwyzard
pushed a commit
that referenced
this pull request
Dec 29, 2020
Remove SiPixelDigiHeterogeneousConverter as obsolete, should have been removed as part of #100. Address review comments for SiPixelClustersCUDA: - remove commented out default constructor and private: from DeviceConstView; this is perhaps the best compromise between non-default constructors not being preferred for device allocations, and the use case in SiPixelRecHitSoAFromLegacy (for the expected life time of this class) - remove const getters with c_ prefix - improve constructor parameter name - use more initializer list - initialize nClusters_h Address review comments for SiPixelDigiErrorsCUDA: - use type alias - remove const getters with c_ prefix and other unnecessary methods - use more initializer list Address review comments for SiPixelDigisCUDA: - remove const getters with c_ prefix and other unnecessary methods - remove commented out default constructor and private: from DeviceConstView - add comments for remaining SiPixelDigisCUDA member arrays Move PixelErrorsCompact and SiPixelDigiErrorsSoa to DataFormats/SiPixelRawData, rename classes Address review comments for SiPixelErrorsSoA - remove redundant assert - move constructor inline Address review comments for SiPixelDigisSoA - remove redundant assert - add comments Enable if constexpr also for CUDA in TrackingRecHit2DHeterogeneous Move dictionary of HostProduct<unsigned int[]> to CUDADataFormats/Common
fwyzard
pushed a commit
that referenced
this pull request
Dec 29, 2020
Remove SiPixelDigiHeterogeneousConverter as obsolete, should have been removed as part of #100. Address review comments for SiPixelClustersCUDA: - remove commented out default constructor and private: from DeviceConstView; this is perhaps the best compromise between non-default constructors not being preferred for device allocations, and the use case in SiPixelRecHitSoAFromLegacy (for the expected life time of this class) - remove const getters with c_ prefix - improve constructor parameter name - use more initializer list - initialize nClusters_h Address review comments for SiPixelDigiErrorsCUDA: - use type alias - remove const getters with c_ prefix and other unnecessary methods - use more initializer list Address review comments for SiPixelDigisCUDA: - remove const getters with c_ prefix and other unnecessary methods - remove commented out default constructor and private: from DeviceConstView - add comments for remaining SiPixelDigisCUDA member arrays Move PixelErrorsCompact and SiPixelDigiErrorsSoa to DataFormats/SiPixelRawData, rename classes Address review comments for SiPixelErrorsSoA - remove redundant assert - move constructor inline Address review comments for SiPixelDigisSoA - remove redundant assert - add comments Enable if constexpr also for CUDA in TrackingRecHit2DHeterogeneous Move dictionary of HostProduct<unsigned int[]> to CUDADataFormats/Common
fwyzard
pushed a commit
that referenced
this pull request
Dec 29, 2020
Remove SiPixelDigiHeterogeneousConverter as obsolete, should have been removed as part of #100. Address review comments for SiPixelClustersCUDA: - remove commented out default constructor and private: from DeviceConstView; this is perhaps the best compromise between non-default constructors not being preferred for device allocations, and the use case in SiPixelRecHitSoAFromLegacy (for the expected life time of this class) - remove const getters with c_ prefix - improve constructor parameter name - use more initializer list - initialize nClusters_h Address review comments for SiPixelDigiErrorsCUDA: - use type alias - remove const getters with c_ prefix and other unnecessary methods - use more initializer list Address review comments for SiPixelDigisCUDA: - remove const getters with c_ prefix and other unnecessary methods - remove commented out default constructor and private: from DeviceConstView - add comments for remaining SiPixelDigisCUDA member arrays Move PixelErrorsCompact and SiPixelDigiErrorsSoa to DataFormats/SiPixelRawData, rename classes Address review comments for SiPixelErrorsSoA - remove redundant assert - move constructor inline Address review comments for SiPixelDigisSoA - remove redundant assert - add comments Enable if constexpr also for CUDA in TrackingRecHit2DHeterogeneous Move dictionary of HostProduct<unsigned int[]> to CUDADataFormats/Common
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description:
This PR addresses to some review comments of cms-sw#31721 (what I've done so far). I could still do some more for the parts of the code I've mostly done, therefore I opened this in draft mode (but I thought there would be some value in an open PR).
(this PR is against
CMSSW_11_2_Patatrackon purpose, I'll cherry pick for a PR againstcms-patatrack:patatrack_integration_9_N_pixel_local_reco"when done")PR validation:
Code compiles.